-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(windows-agent): Remove all writes to registry, and remove virtualization exception #374
Conversation
After an offline discussion we settled for the |
5517680
to
dff833f
Compare
480fb74
to
5a57979
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well done! I could only come up with nitpicks ;)
1f4ab5d
to
ae738d8
Compare
Rebased main for the recent CI fixes. New commits start at fixup: 🌱 Improve E2E error reporting |
ae738d8
to
7764919
Compare
CI failure fixed in #402 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly than the other one, small change request but nothing major.
I trust your tests in the Windows part for virtual registry.
7764919
to
4df74e9
Compare
~~ rebase main 🕺 ~~ |
90% coverage 😎 Also I tested it manually |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM once didrocks comments are addressed and acknowledged.
+1 for the code shrink and the privilege drop!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, thanks for addressing those comments!
af45409
to
17880ae
Compare
Also, the OpenKey function now does not take an access argument. We only ever need read access
This is the standard we've agreed on in the team
This is no longer relevant, as we never write to the registry.
We no longer need this, as we don't write to registry; we only read.
This field no longer exists
This field no longer exists. The test checking this field was also removed.
Now that the write permissions to the registry are irrelevant, we need a way for the sysadmin to enforce its configuration on the machine. To achieve this, we set the org-provided Landscape and Pro settings as the top layer.
17880ae
to
f6f239a
Compare
Rebased main after #404 now that the CI errors should be fixed. |
With the recent changes in #369, we no longer need to write to the registry.
There is one behavior change here: setting the registry as read-only no longer has any effect on the program. Until now, it was an implicit message from the administrator to UP4W to block attempts to modify the config.
UDENG-1696